Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

tls: refactoring SNI ctx usage for libressl support #1136

Merged
merged 9 commits into from
Jun 6, 2024
Merged

Conversation

sreimers
Copy link
Member

@sreimers sreimers commented Jun 3, 2024

Replaces unsupported (by LibreSSL) SSL_certs_clear with recommended SSL_set_SSL_CTX():

https://www.openssl.org/docs/man3.3/man3/SSL_CTX_set_tlsext_servername_callback.html

The servername callback should return one of the following values:

SSL_TLSEXT_ERR_OK
This is used to indicate that the servername requested by the client has been accepted. Typically a server will call SSL_set_SSL_CTX() in the callback to set up a different configuration for the selected servername in this case.

@sreimers
Copy link
Member Author

sreimers commented Jun 3, 2024

@cspiel1 can you review and test with your SNI setup?

@cspiel1
Copy link
Collaborator

cspiel1 commented Jun 4, 2024

Yes, I'll try to do so. It will take some time to arrange the setup. Maybe I have to ask also our QA team.

@sreimers sreimers marked this pull request as ready for review June 5, 2024 08:25
@cspiel1
Copy link
Collaborator

cspiel1 commented Jun 5, 2024

By means of debugging I could verify that the correct certificate was selected. But then I had no luck with the cert verification on the callee (TLS server side), also in main branch. The next two days will be difficult to work on this.

src/tls/openssl/sni.c Outdated Show resolved Hide resolved
@cspiel1
Copy link
Collaborator

cspiel1 commented Jun 6, 2024

I am coming closer with the test setup. Will have a result soon.

@sreimers sreimers changed the title tls: refactoring sni ctx usage for libressl support tls: refactoring SNI ctx usage for libressl support Jun 6, 2024
@cspiel1
Copy link
Collaborator

cspiel1 commented Jun 6, 2024

I have some troubles:

  • The callee (TLS server) seems to verify the client certificate incl host name check. sip_verify_client no seems to make no difference.
  • It seems that the setting sip_capath does not work.

@sreimers
Copy link
Member Author

sreimers commented Jun 6, 2024

  • The callee (TLS server) seems to verify the client certificate incl host name check. sip_verify_client no seems to make no difference.

Not sure why, but verify client is always enabled for SNI here:

SSL_set_verify(ssl, SSL_VERIFY_PEER, tls_verify_handler);

  • It seems that the setting sip_capath does not work.

It works for test, does it also not work with main?

@cspiel1
Copy link
Collaborator

cspiel1 commented Jun 6, 2024

Beside of these issues, that also occur in main branch this PR seems to be okay. SNI works.

tls: tls_verify_handler: subject_name = /C=AU/ST=Some-State/O=Retest/CN=Mr Retest Root CA
tls: tls_verify_handler: issuer_name  = /C=AU/ST=Some-State/O=Retest/CN=Mr Retest Root CA
tls: tls verify ok = 1
tls: tls_verify_handler: subject_name = /C=AU/ST=Some-State/O=Retest/CN=Mr Retest Intermediate CA
tls: tls_verify_handler: issuer_name  = /C=AU/ST=Some-State/O=Retest/CN=Mr Retest Root CA
tls: tls verify ok = 1
tls: tls_verify_handler: subject_name = /C=AU/ST=Some-State/O=Retest/CN=server.foo.local
tls: tls_verify_handler: issuer_name  = /C=AU/ST=Some-State/O=Retest/CN=Mr Retest Intermediate CA
tls: tls verify ok = 1

With another cert chain, and a second account it works also. But as mentioned above, the sip_cafile has to be set to the second chain.

The code looks good. I suggest to merge this.

@cspiel1
Copy link
Collaborator

cspiel1 commented Jun 6, 2024

Not sure why, but verify client is always enabled for SNI here:

Ah, ok. Not sure if this is wanted.

It works for test, does it also not work with main?

Yes. This should be analysed further.

@sreimers sreimers merged commit ef244d6 into main Jun 6, 2024
38 checks passed
@sreimers sreimers deleted the libressl_sni branch June 6, 2024 13:28
@maximilianfridrich
Copy link
Contributor

@sreimers I have noticed that this PR changed the behavior quite a bit. Was that on purpose?

E.g. if the peer does not add the server_name extension, then the handshake always fails now. Before, the handshake proceeded.

@cspiel1
Copy link
Collaborator

cspiel1 commented Jul 30, 2024

There are different options to choose a certificate. E.g. Certificates configured in accounts. Or globally in config file.

Thus a missing server_name should not be the reason that TLS handshake fails. As soon as a wrong certificate or no certificate is offered the TLS handshake should fail.

@sreimers
Copy link
Member Author

@sreimers I have noticed that this PR changed the behavior quite a bit. Was that on purpose?

I think this is a bug, can you try:

diff --git a/src/tls/openssl/sni.c b/src/tls/openssl/sni.c
index 8298e40f..8be0e7f7 100644
--- a/src/tls/openssl/sni.c
+++ b/src/tls/openssl/sni.c
@@ -166,10 +166,8 @@ static int ssl_servername_handler(SSL *ssl, int *al, void *arg)
        const char *sni;
 
        sni = SSL_get_servername(ssl, TLSEXT_NAMETYPE_host_name);
-       if (!str_isset(sni)) {
-               *al = SSL_AD_UNRECOGNIZED_NAME;
-               return SSL_TLSEXT_ERR_ALERT_FATAL;
-       }
+       if (!str_isset(sni))
+               return SSL_TLSEXT_ERR_OK;
 
        /* find and apply matching certificate */
        uc = tls_cert_for_sni(tls, sni);

@maximilianfridrich
Copy link
Contributor

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants